Zush/cli recording#555
Conversation
📝 WalkthroughWalkthroughThis PR adds a complete CLI tooling layer for recording screens and rendering/exporting projects via a Node executable ( ChangesCLI Recording & Rendering Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
Review notesHeads up on a few things while you're diving in at 2am: CLI Script (666 lines) — That's a lot of surface area. The flag parsing and validation helpers are kinda repetitive but consistent; makes sense for a single entry point. The Electron Main Changes — The split between Renderer Components — Minor consideration: The 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db9e9b8d48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const format = | ||
| parseExportFormat(options.format) ?? | ||
| (extension === ".gif" ? "gif" : project.editor.exportFormat); |
There was a problem hiding this comment.
When the user omits --format, every non-.gif output falls back to project.editor.exportFormat, so a project saved with exportFormat: "gif" and rendered as openscreen render --output out.mp4 writes GIF bytes to an .mp4 file. Since the CLI advertises defaulting from the output extension, infer .mp4 explicitly before using the project default to avoid mislabeled/unplayable exports.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
electron/main.ts (2)
562-589: 💤 Low value
webSecurity: falseon the cli windows — worth a comment explaining why.i'm assuming this is needed so the renderer can
file://-fetch local recordings/cursor telemetry without CORS getting in the way. that's defensible because the only content loaded is your own dev server / packaged renderer + user-supplied local files, but this is the kind of thing that screams "lowkey suspicious" in a security review six months from now.drop a one-liner above the
webPreferencesblock explaining the rationale, so future-you doesn't have to reverse-engineer it. same applies to the render window at line ~692.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main.ts` around lines 562 - 589, Add a one-line comment above the webPreferences block in the BrowserWindow construction (the object passed to new BrowserWindow in main.ts) explaining why webSecurity is set to false (e.g., to allow the renderer to load local file:// recordings/cursor telemetry without CORS issues while only loading our own dev/prod renderer plus user files). Do the same next to the other BrowserWindow creation used for the render window later in the file so future reviewers understand the security tradeoff and context; reference the webPreferences.webSecurity setting and the preload/path loading logic in your comment for clarity.
522-545: 💤 Low value
createHiddenWindowre-used as both the source-selector and countdown-overlay factory is a footgun.if anything ever invokes
countdown-overlay-showoropen-source-selectorfrom the cli renderer (today: nothing does, butregisterIpcHandlersexposes those channels), you'll get freshly-created hidden BrowserWindows that nobody owns or destroys. probably never triggered in practice, but it's the kind of thing that bites in two years when someone refactors a shared util.a defensive
() => { throw new Error("not supported in cli mode"); }factory would make the contract explicit. nit-level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main.ts` around lines 522 - 545, The CLI-mode IPC factories currently reuse createHiddenWindow for source-selector and countdown-overlay which can create unowned BrowserWindows if those channels are ever invoked; replace the createHiddenWindow usages passed into registerIpcHandlers for the source-selector and countdown-overlay slots with defensive factories that throw a clear Error (e.g., "() => { throw new Error('not supported in cli mode') }") so any call to the channels like "countdown-overlay-show" or "open-source-selector" from the CLI renderer fails fast and makes the contract explicit; update the arguments passed to registerIpcHandlers (preserving createHiddenWindow where appropriate) to use those throwing factories for the CLI-only slots (referencing createHiddenWindow and the registerIpcHandlers call to locate the change).src/components/cli-render/CliRenderRenderer.tsx (1)
226-235: 💤 Low valuenit:
onProgresscould spam stdout on long renders.every progress tick gets serialized to JSON and written to stdout via the cli message channel. for a 10-minute render that's potentially thousands of lines. consider throttling (e.g. only emit when
progress.percentchanges by ≥1, or rate-limit to ~5/s) to keep the CLI output sane and reduce IPC chatter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/cli-render/CliRenderRenderer.tsx` around lines 226 - 235, The onProgress handler currently forwards every ExportProgress tick via sendToMain (used when building base config with buildBaseConfig), which can flood stdout during long renders; modify the onProgress implementation to throttle emissions — e.g., keep a small local state (lastPercent and lastEmitTime) and only call sendToMain("progress", progress) when progress.percent has changed by >=1 or when at least ~200ms has elapsed (≈5/s), and ensure you reference the same onProgress passed into buildBaseConfig so all progress events are filtered before IPC.src/components/cli-record/CliRecordRenderer.tsx (2)
167-172: 💤 Low valuefixed-duration sleep doesn't react to recorder errors.
await new Promise(r => setTimeout(r, durationMs))will happily wait the full requested duration even if theMediaRecorderalready rejectedrecordedBlobPromise(e.g. track ended, encoder hiccup). on a 30+min cli record that's a lot of dead waiting before the safety timer in main.ts kicks in.consider racing the timeout against
recordedBlobPromise.catch()so we bail early on encoder failure. nit, not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/cli-record/CliRecordRenderer.tsx` around lines 167 - 172, The fixed-duration sleep currently always waits config.durationMs even if recorderHandle.recordedBlobPromise rejects; update the logic around the Promise that waits for config.durationMs (the block using startedAt and the setTimeout) to race the timeout against recorderHandle.recordedBlobPromise.catch(...) or recorderHandle.recordedBlobPromise itself so that on encoder/recorder errors we stop waiting and proceed immediately to recorderHandle.recorder.stop() and handling recordedBlobPromise rejection before calling fixWebmDuration(recordedBlob, durationMs); ensure durationMs still reflects actual elapsed time (use Date.now() - startedAt) when the race completes.
44-54: 💤 Low valueswap vp8 and vp9 in the preference order.
vp9 is the preferred webm codec (better quality-per-bit, widely supported), so it should come before vp8. the web search confirms vp9→vp8 is the standard fallback chain—firefox historically lacked vp9 support, so this ordering ensures decent coverage. lowkey the current order inverts what mediarecorder expects to try first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/cli-record/CliRecordRenderer.tsx` around lines 44 - 54, The preferred MIME types array in selectMimeType lists vp8 before vp9; swap "video/webm;codecs=vp8" and "video/webm;codecs=vp9" so vp9 is tried before vp8 (i.e., preferred = ["video/webm;codecs=h264","video/webm;codecs=vp9","video/webm;codecs=vp8","video/webm;codecs=av1","video/webm"]) to reflect the intended vp9→vp8 fallback order used by MediaRecorder.isTypeSupported.electron/electron-env.d.ts (1)
148-173: 💤 Low valuenit: these CLI config types are duplicated in
electron/main.tsand re-derived inCliRenderRenderer.tsx.three places defining the same shape is kinda asking for drift. consider hoisting
CliRecordConfig/CliRenderConfiginto a shared types module (e.g.src/lib/cliTypes.ts) and importing them inelectron/main.ts,electron/electron-env.d.ts, and the renderers. not a blocker — the shapes currently match — just future-proofing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/electron-env.d.ts` around lines 148 - 173, Extract the duplicated CLI types into a single exported type/module (e.g. export interfaces CliRecordConfig and CliRenderConfig) and import those types where the shapes are currently re-defined: replace the inline return types of getCliRecordConfig and getCliRenderConfig in electron-env.d.ts, the corresponding definitions in electron/main.ts, and the types used in CliRenderRenderer.tsx with references to the shared CliRecordConfig and CliRenderConfig. Ensure the shared module exports the exact fields currently used (durationMs, source, sourceType, systemAudio, and the full project/output/format/quality/size/frameRate/gifLoop shape) and update any callsites to use the imported types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/openscreen.mjs`:
- Around line 533-536: The CLI currently accepts --id verbatim which allows
duplicate region ids and makes remove/edit ambiguous; update the add path (where
id is read with requireOption and regions is the array) to reject duplicates by
checking if regions.some(region => region.id === id) and throwing a clear Error
if found, so new regions cannot use an existing id. Also add the same uniqueness
check where new ids are accepted (the add-related block around lines handling
action === "add" and the code referenced at 547-549) so duplicates are prevented
consistently; you can keep remove's findIndex logic but only after ensuring ids
are unique on add.
- Around line 591-595: The code that infers format from the outputPath only
special-cases ".gif", so when outputPath ends with ".mp4" it still uses
project.editor.exportFormat; update the format inference (around the extension /
format assignment using parseExportFormat, extension, and
project.editor.exportFormat) to explicitly map ".mp4" to "mp4" (e.g., extension
=== ".gif" ? "gif" : extension === ".mp4" ? "mp4" : project.editor.exportFormat)
so that if parseExportFormat(options.format) is undefined the extension drives
the export format correctly for .mp4 outputs; leave parseExportQuality and other
logic unchanged.
- Around line 301-314: The CLI currently parses numeric flags but doesn't
enforce documented ranges, so add explicit range validation after calling
parseNumber for the "padding" and "motion-blur" flags: when handling
options.padding (which sets updates.padding) ensure the parsed value is finite
and within 0–100 and when handling options["motion-blur"] (which sets
updates.motionBlurAmount) ensure the parsed value is finite and within 0–1; if a
value is outside the allowed range, return or throw a clear CLI error (or exit
with a user-facing message) indicating the expected range so invalid values are
not persisted.
- Around line 433-438: The record branch that handles options.output currently
always overwrites existing files; mirror the render logic by checking for an
existing target and honoring the overwrite flag (e.g.
options.overwrite/--overwrite) before copying. In the block that uses outputPath
and done.path, add an existence check (fs.existsSync(outputPath) or equivalent)
and if the file exists and overwrite is not set, abort/throw/report an error;
only call ensureParentDir, fs.copyFileSync(done.path, outputPath) and
copyCompanionFile(done.path, outputPath, ".cursor.json") when the check permits
overwriting. Ensure both the main file (outputPath) and its companion are
guarded the same way.
- Around line 352-379: The stdout data handler for child.stdout (the
child.stdout.on("data", ...) callback) assumes each chunk ends with a newline
and can drop split JSON lines; fix by adding a persistent buffer (e.g.,
stdoutBuffer = "") that you append chunk.toString() to, then split buffer by
"\n", keep the last element as the remainder (do not try to JSON.parse it if
buffer did not end with a newline), parse only the complete lines and update
done/lastError/progress/status as before, and on child.stdout 'end' or child
'close' flush any remaining buffer attempt to parse the final line; this
prevents partial JSON messages (like the done packet) from being swallowed by
the catch block.
In `@electron/main.ts`:
- Around line 624-637: The returned CLI render config currently trusts parsed
values; add a small validation block before returning that: validate width and
height by coercing parsed.width/parsed.height to Number, ensure Number.isFinite
and >= 2, then floor to the nearest even integer (e.g. Math.max(2,
Math.floor(num/2)*2)) or set undefined if not provided; validate gifFrameRate by
coercing to Number and clamping/whitelisting to one of [15,20,25,30] (or throw a
descriptive Error if out of range); coerce gifLoop to a strict boolean
(Boolean(parsed.gifLoop)); keep existing enum checks for format, quality,
gifSizePreset but throw a clear error when values are invalid so the caller
fails fast rather than letting bad width/height/gifFrameRate/gifLoop propagate
to downstream functions that use project, output, format, quality, width,
height, gifFrameRate, gifSizePreset, gifLoop.
- Around line 714-736: The CLI paths (isCliRecord / isCliRender) return before
registering session.defaultSession.setPermissionCheckHandler and
setPermissionRequestHandler and before calling
systemPreferences.askForMediaAccess("microphone"), causing macOS first-run
permission issues; move or duplicate the permission handler registration and the
microphone pre-prompt into the CLI branch before calling
runCliRecord/runCliRender (or extract to a helper used by both GUI and CLI), and
if the runCliRecord path detects the systemAudio flag, explicitly call
systemPreferences.askForMediaAccess("microphone") (and surface failures via
writeCliMessage("error", ...)) before awaiting runCliRecord so the OS pre-prompt
occurs when running headless.
In `@src/components/cli-record/CliRecordRenderer.tsx`:
- Around line 121-203: The finally block can skip stopping the MediaRecorder if
an error occurs earlier, so update the cleanup to first check recorderHandle and
its recorder state (e.g., recorderHandle.recorder &&
recorderHandle.recorder.state !== "inactive") and call
recorderHandle.recorder.stop() inside a try/catch that swallows
InvalidStateError (and other errors) before stopping media tracks; then await
the recorderHandle.recordedBlobPromise (or handle its rejection) before calling
mediaStream.getTracks().forEach(track => track.stop()) and still call
window.electronAPI.setRecordingState(false) in a safe try/catch—this ensures
recorder.stop() runs defensively and the final blob gets flushed even on error
paths.
In `@src/components/cli-render/CliRenderRenderer.tsx`:
- Around line 238-258: The GIF branch is ignoring CLI width/height flags, so
update the gif handling in CliRenderRenderer.tsx to honor user overrides: when
config.format === "gif" read config.width and config.height (falling back to
calculateOutputDimensions like currently done) and pass the resolved
width/height into the new GifExporter instead of only using gifSizePreset;
reference calculateOutputDimensions and GifExporter and use
config.width/config.height as overrides for dimensions.width/dimensions.height.
Alternatively (or additionally) if you prefer not to override presets, detect
when config.width or config.height is set with format "gif" and call
sendToMain("warning", ...) to surface a warning to the user that CLI
width/height are ignored for GIFs.
---
Nitpick comments:
In `@electron/electron-env.d.ts`:
- Around line 148-173: Extract the duplicated CLI types into a single exported
type/module (e.g. export interfaces CliRecordConfig and CliRenderConfig) and
import those types where the shapes are currently re-defined: replace the inline
return types of getCliRecordConfig and getCliRenderConfig in electron-env.d.ts,
the corresponding definitions in electron/main.ts, and the types used in
CliRenderRenderer.tsx with references to the shared CliRecordConfig and
CliRenderConfig. Ensure the shared module exports the exact fields currently
used (durationMs, source, sourceType, systemAudio, and the full
project/output/format/quality/size/frameRate/gifLoop shape) and update any
callsites to use the imported types.
In `@electron/main.ts`:
- Around line 562-589: Add a one-line comment above the webPreferences block in
the BrowserWindow construction (the object passed to new BrowserWindow in
main.ts) explaining why webSecurity is set to false (e.g., to allow the renderer
to load local file:// recordings/cursor telemetry without CORS issues while only
loading our own dev/prod renderer plus user files). Do the same next to the
other BrowserWindow creation used for the render window later in the file so
future reviewers understand the security tradeoff and context; reference the
webPreferences.webSecurity setting and the preload/path loading logic in your
comment for clarity.
- Around line 522-545: The CLI-mode IPC factories currently reuse
createHiddenWindow for source-selector and countdown-overlay which can create
unowned BrowserWindows if those channels are ever invoked; replace the
createHiddenWindow usages passed into registerIpcHandlers for the
source-selector and countdown-overlay slots with defensive factories that throw
a clear Error (e.g., "() => { throw new Error('not supported in cli mode') }")
so any call to the channels like "countdown-overlay-show" or
"open-source-selector" from the CLI renderer fails fast and makes the contract
explicit; update the arguments passed to registerIpcHandlers (preserving
createHiddenWindow where appropriate) to use those throwing factories for the
CLI-only slots (referencing createHiddenWindow and the registerIpcHandlers call
to locate the change).
In `@src/components/cli-record/CliRecordRenderer.tsx`:
- Around line 167-172: The fixed-duration sleep currently always waits
config.durationMs even if recorderHandle.recordedBlobPromise rejects; update the
logic around the Promise that waits for config.durationMs (the block using
startedAt and the setTimeout) to race the timeout against
recorderHandle.recordedBlobPromise.catch(...) or
recorderHandle.recordedBlobPromise itself so that on encoder/recorder errors we
stop waiting and proceed immediately to recorderHandle.recorder.stop() and
handling recordedBlobPromise rejection before calling
fixWebmDuration(recordedBlob, durationMs); ensure durationMs still reflects
actual elapsed time (use Date.now() - startedAt) when the race completes.
- Around line 44-54: The preferred MIME types array in selectMimeType lists vp8
before vp9; swap "video/webm;codecs=vp8" and "video/webm;codecs=vp9" so vp9 is
tried before vp8 (i.e., preferred =
["video/webm;codecs=h264","video/webm;codecs=vp9","video/webm;codecs=vp8","video/webm;codecs=av1","video/webm"])
to reflect the intended vp9→vp8 fallback order used by
MediaRecorder.isTypeSupported.
In `@src/components/cli-render/CliRenderRenderer.tsx`:
- Around line 226-235: The onProgress handler currently forwards every
ExportProgress tick via sendToMain (used when building base config with
buildBaseConfig), which can flood stdout during long renders; modify the
onProgress implementation to throttle emissions — e.g., keep a small local state
(lastPercent and lastEmitTime) and only call sendToMain("progress", progress)
when progress.percent has changed by >=1 or when at least ~200ms has elapsed
(≈5/s), and ensure you reference the same onProgress passed into buildBaseConfig
so all progress events are filtered before IPC.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28eebc02-467f-4588-b7f3-7175b2ca09fe
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
cli/openscreen.mjselectron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tspackage.jsonsrc/App.tsxsrc/components/cli-record/CliRecordRenderer.tsxsrc/components/cli-render/CliRenderRenderer.tsx
| const aspectRatio = parseAspectRatio(options["aspect-ratio"]); | ||
| if (aspectRatio) updates.aspectRatio = aspectRatio; | ||
| if (typeof options.wallpaper === "string") updates.wallpaper = options.wallpaper; | ||
| if (options.padding !== undefined) updates.padding = parseNumber(options.padding, "padding"); | ||
| if (options["border-radius"] !== undefined) { | ||
| updates.borderRadius = parseNumber(options["border-radius"], "border-radius"); | ||
| } | ||
| if (options["shadow-intensity"] !== undefined) { | ||
| updates.shadowIntensity = parseNumber(options["shadow-intensity"], "shadow-intensity"); | ||
| } | ||
| if (options["show-blur"] !== undefined) updates.showBlur = Boolean(options["show-blur"]); | ||
| if (options["motion-blur"] !== undefined) { | ||
| updates.motionBlurAmount = parseNumber(options["motion-blur"], "motion-blur"); | ||
| } |
There was a problem hiding this comment.
Validate the documented editor ranges before saving them.
--padding is documented as 0-100 and --motion-blur as 0-1, but this accepts any finite number and persists it into the project. that makes the CLI help lie and lets invalid state leak into later renders.
Possible fix
const updates = {};
const aspectRatio = parseAspectRatio(options["aspect-ratio"]);
if (aspectRatio) updates.aspectRatio = aspectRatio;
if (typeof options.wallpaper === "string") updates.wallpaper = options.wallpaper;
- if (options.padding !== undefined) updates.padding = parseNumber(options.padding, "padding");
+ if (options.padding !== undefined) {
+ const padding = parseNumber(options.padding, "padding");
+ if (padding < 0 || padding > 100) {
+ throw new Error("--padding must be between 0 and 100.");
+ }
+ updates.padding = padding;
+ }
if (options["border-radius"] !== undefined) {
updates.borderRadius = parseNumber(options["border-radius"], "border-radius");
}
@@
- if (options["motion-blur"] !== undefined) {
- updates.motionBlurAmount = parseNumber(options["motion-blur"], "motion-blur");
- }
+ if (options["motion-blur"] !== undefined) {
+ const motionBlurAmount = parseNumber(options["motion-blur"], "motion-blur");
+ if (motionBlurAmount < 0 || motionBlurAmount > 1) {
+ throw new Error("--motion-blur must be between 0 and 1.");
+ }
+ updates.motionBlurAmount = motionBlurAmount;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const aspectRatio = parseAspectRatio(options["aspect-ratio"]); | |
| if (aspectRatio) updates.aspectRatio = aspectRatio; | |
| if (typeof options.wallpaper === "string") updates.wallpaper = options.wallpaper; | |
| if (options.padding !== undefined) updates.padding = parseNumber(options.padding, "padding"); | |
| if (options["border-radius"] !== undefined) { | |
| updates.borderRadius = parseNumber(options["border-radius"], "border-radius"); | |
| } | |
| if (options["shadow-intensity"] !== undefined) { | |
| updates.shadowIntensity = parseNumber(options["shadow-intensity"], "shadow-intensity"); | |
| } | |
| if (options["show-blur"] !== undefined) updates.showBlur = Boolean(options["show-blur"]); | |
| if (options["motion-blur"] !== undefined) { | |
| updates.motionBlurAmount = parseNumber(options["motion-blur"], "motion-blur"); | |
| } | |
| const aspectRatio = parseAspectRatio(options["aspect-ratio"]); | |
| if (aspectRatio) updates.aspectRatio = aspectRatio; | |
| if (typeof options.wallpaper === "string") updates.wallpaper = options.wallpaper; | |
| if (options.padding !== undefined) { | |
| const padding = parseNumber(options.padding, "padding"); | |
| if (padding < 0 || padding > 100) { | |
| throw new Error("--padding must be between 0 and 100."); | |
| } | |
| updates.padding = padding; | |
| } | |
| if (options["border-radius"] !== undefined) { | |
| updates.borderRadius = parseNumber(options["border-radius"], "border-radius"); | |
| } | |
| if (options["shadow-intensity"] !== undefined) { | |
| updates.shadowIntensity = parseNumber(options["shadow-intensity"], "shadow-intensity"); | |
| } | |
| if (options["show-blur"] !== undefined) updates.showBlur = Boolean(options["show-blur"]); | |
| if (options["motion-blur"] !== undefined) { | |
| const motionBlurAmount = parseNumber(options["motion-blur"], "motion-blur"); | |
| if (motionBlurAmount < 0 || motionBlurAmount > 1) { | |
| throw new Error("--motion-blur must be between 0 and 1."); | |
| } | |
| updates.motionBlurAmount = motionBlurAmount; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openscreen.mjs` around lines 301 - 314, The CLI currently parses numeric
flags but doesn't enforce documented ranges, so add explicit range validation
after calling parseNumber for the "padding" and "motion-blur" flags: when
handling options.padding (which sets updates.padding) ensure the parsed value is
finite and within 0–100 and when handling options["motion-blur"] (which sets
updates.motionBlurAmount) ensure the parsed value is finite and within 0–1; if a
value is outside the allowed range, return or throw a clear CLI error (or exit
with a user-facing message) indicating the expected range so invalid values are
not persisted.
| child.stdout.on("data", (chunk) => { | ||
| for (const line of chunk.toString().split("\n").filter(Boolean)) { | ||
| try { | ||
| const message = JSON.parse(line); | ||
| if (!message.__cli) continue; | ||
|
|
||
| if (message.type === "status" && !options.json) { | ||
| process.stderr.write(`${message.data?.message ?? ""}\n`); | ||
| } | ||
| if (message.type === "warning" && !options.json) { | ||
| process.stderr.write(`Warning: ${message.data?.message ?? ""}\n`); | ||
| } | ||
| if (message.type === "progress" && !options.json) { | ||
| const current = Number(message.data?.currentFrame ?? 0); | ||
| const total = Number(message.data?.totalFrames ?? 0); | ||
| if (total > 0) { | ||
| process.stderr.write(`\r${Math.round((current / total) * 100)}%`); | ||
| } | ||
| } | ||
| if (message.type === "done") done = message.data; | ||
| if (message.type === "error") { | ||
| lastError = message.data?.message ?? `Unknown ${mode} error`; | ||
| } | ||
| } catch { | ||
| // Ignore non-JSON logs from Electron. | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Buffer stdout before parsing __cli messages.
This handler assumes each data event ends on a newline. streams don’t promise that, so a split JSON line gets dropped by the catch {} path. if the done packet is the one that gets split, the child can exit 0 and we still reject. lowkey risky.
Possible fix
+ let stdoutBuffer = "";
child.stdout.on("data", (chunk) => {
- for (const line of chunk.toString().split("\n").filter(Boolean)) {
+ stdoutBuffer += chunk.toString();
+ const lines = stdoutBuffer.split("\n");
+ stdoutBuffer = lines.pop() ?? "";
+ for (const line of lines.filter(Boolean)) {
try {
const message = JSON.parse(line);
if (!message.__cli) continue;
@@
// Ignore non-JSON logs from Electron.
}
}
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openscreen.mjs` around lines 352 - 379, The stdout data handler for
child.stdout (the child.stdout.on("data", ...) callback) assumes each chunk ends
with a newline and can drop split JSON lines; fix by adding a persistent buffer
(e.g., stdoutBuffer = "") that you append chunk.toString() to, then split buffer
by "\n", keep the last element as the remainder (do not try to JSON.parse it if
buffer did not end with a newline), parse only the complete lines and update
done/lastError/progress/status as before, and on child.stdout 'end' or child
'close' flush any remaining buffer attempt to parse the final line; this
prevents partial JSON messages (like the done packet) from being swallowed by
the catch block.
| if (options.output) { | ||
| const outputPath = path.resolve(options.output); | ||
| ensureParentDir(outputPath); | ||
| fs.copyFileSync(done.path, outputPath); | ||
| copyCompanionFile(done.path, outputPath, ".cursor.json"); | ||
| done.path = outputPath; |
There was a problem hiding this comment.
Guard record --output with the same overwrite check as render.
This path always copies into outputPath, so an existing recording and its .cursor.json companion get replaced silently. render already blocks that unless --overwrite is set; record should too.
Possible fix
if (options.output) {
const outputPath = path.resolve(options.output);
+ if (fs.existsSync(outputPath) && !options.overwrite) {
+ throw new Error(`Output file already exists: ${outputPath}. Use --overwrite to replace.`);
+ }
ensureParentDir(outputPath);
fs.copyFileSync(done.path, outputPath);
copyCompanionFile(done.path, outputPath, ".cursor.json");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openscreen.mjs` around lines 433 - 438, The record branch that handles
options.output currently always overwrites existing files; mirror the render
logic by checking for an existing target and honoring the overwrite flag (e.g.
options.overwrite/--overwrite) before copying. In the block that uses outputPath
and done.path, add an existence check (fs.existsSync(outputPath) or equivalent)
and if the file exists and overwrite is not set, abort/throw/report an error;
only call ensureParentDir, fs.copyFileSync(done.path, outputPath) and
copyCompanionFile(done.path, outputPath, ".cursor.json") when the check permits
overwriting. Ensure both the main file (outputPath) and its companion are
guarded the same way.
| if (action === "remove") { | ||
| const id = requireOption(options, "id"); | ||
| const index = regions.findIndex((region) => region.id === id); | ||
| if (index === -1) throw new Error(`Region not found: ${id}`); |
There was a problem hiding this comment.
Reject duplicate region ids on add.
--id is accepted verbatim, but remove only deletes the first match. duplicate ids make later edits/removals ambiguous and can leave one of the regions effectively unreachable from the CLI. kinda cursed once a project has two zoom-7s.
Possible fix
const range = parseRange(options);
const id = options.id ?? deriveNextId(kind, regions);
+ if (regions.some((region) => region.id === id)) {
+ throw new Error(`${kind} region id already exists: ${id}`);
+ }
let region;Also applies to: 547-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openscreen.mjs` around lines 533 - 536, The CLI currently accepts --id
verbatim which allows duplicate region ids and makes remove/edit ambiguous;
update the add path (where id is read with requireOption and regions is the
array) to reject duplicates by checking if regions.some(region => region.id ===
id) and throwing a clear Error if found, so new regions cannot use an existing
id. Also add the same uniqueness check where new ids are accepted (the
add-related block around lines handling action === "add" and the code referenced
at 547-549) so duplicates are prevented consistently; you can keep remove's
findIndex logic but only after ensuring ids are unique on add.
| const extension = path.extname(outputPath).toLowerCase(); | ||
| const format = | ||
| parseExportFormat(options.format) ?? | ||
| (extension === ".gif" ? "gif" : project.editor.exportFormat); | ||
| const quality = parseExportQuality(options.quality) ?? project.editor.exportQuality; |
There was a problem hiding this comment.
Honor .mp4 when inferring the render format.
Right now only .gif overrides the project default. if a project default is GIF and the user runs render --output clip.mp4, this still renders GIF data to an .mp4 filename. downstream tooling is gonna have a bad time.
Possible fix
const extension = path.extname(outputPath).toLowerCase();
const format =
parseExportFormat(options.format) ??
- (extension === ".gif" ? "gif" : project.editor.exportFormat);
+ (extension === ".gif"
+ ? "gif"
+ : extension === ".mp4"
+ ? "mp4"
+ : project.editor.exportFormat);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const extension = path.extname(outputPath).toLowerCase(); | |
| const format = | |
| parseExportFormat(options.format) ?? | |
| (extension === ".gif" ? "gif" : project.editor.exportFormat); | |
| const quality = parseExportQuality(options.quality) ?? project.editor.exportQuality; | |
| const extension = path.extname(outputPath).toLowerCase(); | |
| const format = | |
| parseExportFormat(options.format) ?? | |
| (extension === ".gif" | |
| ? "gif" | |
| : extension === ".mp4" | |
| ? "mp4" | |
| : project.editor.exportFormat); | |
| const quality = parseExportQuality(options.quality) ?? project.editor.exportQuality; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openscreen.mjs` around lines 591 - 595, The code that infers format from
the outputPath only special-cases ".gif", so when outputPath ends with ".mp4" it
still uses project.editor.exportFormat; update the format inference (around the
extension / format assignment using parseExportFormat, extension, and
project.editor.exportFormat) to explicitly map ".mp4" to "mp4" (e.g., extension
=== ".gif" ? "gif" : extension === ".mp4" ? "mp4" : project.editor.exportFormat)
so that if parseExportFormat(options.format) is undefined the extension drives
the export format correctly for .mp4 outputs; leave parseExportQuality and other
logic unchanged.
| return { | ||
| project: parsed.project as CliRenderConfig["project"], | ||
| output, | ||
| format: parsed.format === "gif" ? "gif" : "mp4", | ||
| quality: parsed.quality === "medium" || parsed.quality === "source" ? parsed.quality : "good", | ||
| width: Number.isFinite(parsed.width) ? Math.floor(Number(parsed.width) / 2) * 2 : undefined, | ||
| height: Number.isFinite(parsed.height) ? Math.floor(Number(parsed.height) / 2) * 2 : undefined, | ||
| gifFrameRate: parsed.gifFrameRate, | ||
| gifSizePreset: | ||
| parsed.gifSizePreset === "large" || parsed.gifSizePreset === "original" | ||
| ? parsed.gifSizePreset | ||
| : "medium", | ||
| gifLoop: parsed.gifLoop, | ||
| }; |
There was a problem hiding this comment.
input validation is a bit loose for cli render config.
a few things sneaking through here:
width/height:Math.floor(Number(parsed.width) / 2) * 2accepts0, negatives, andNaN-after-coercion. a width of0will silently propagate to the renderer'scalculateCustomMp4Dimensions(0, 0)and then to aVideoExportercall that'll fail somewhere downstream with a less obvious error.gifFrameRate: passed through with zero validation. user suppliesgifFrameRate: 999, it gets cast toGifFrameRatein the renderer. better to clamp to the15 | 20 | 25 | 30set or reject.gifLoop: same — accepts any truthy/falsy without coercing to boolean.
a small validator block here would fail fast with a useful message instead of crashing in the renderer:
🛡️ proposed validation
+const VALID_GIF_FRAME_RATES = new Set([15, 20, 25, 30]);
+
+function parseDimension(value: unknown): number | undefined {
+ if (typeof value !== "number" || !Number.isFinite(value) || value < 2) return undefined;
+ return Math.floor(value / 2) * 2;
+}
+
return {
project: parsed.project as CliRenderConfig["project"],
output,
format: parsed.format === "gif" ? "gif" : "mp4",
quality: parsed.quality === "medium" || parsed.quality === "source" ? parsed.quality : "good",
- width: Number.isFinite(parsed.width) ? Math.floor(Number(parsed.width) / 2) * 2 : undefined,
- height: Number.isFinite(parsed.height) ? Math.floor(Number(parsed.height) / 2) * 2 : undefined,
- gifFrameRate: parsed.gifFrameRate,
+ width: parseDimension(parsed.width),
+ height: parseDimension(parsed.height),
+ gifFrameRate: VALID_GIF_FRAME_RATES.has(Number(parsed.gifFrameRate))
+ ? (parsed.gifFrameRate as 15 | 20 | 25 | 30)
+ : undefined,
gifSizePreset:
parsed.gifSizePreset === "large" || parsed.gifSizePreset === "original"
? parsed.gifSizePreset
: "medium",
- gifLoop: parsed.gifLoop,
+ gifLoop: typeof parsed.gifLoop === "boolean" ? parsed.gifLoop : undefined,
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/main.ts` around lines 624 - 637, The returned CLI render config
currently trusts parsed values; add a small validation block before returning
that: validate width and height by coercing parsed.width/parsed.height to
Number, ensure Number.isFinite and >= 2, then floor to the nearest even integer
(e.g. Math.max(2, Math.floor(num/2)*2)) or set undefined if not provided;
validate gifFrameRate by coercing to Number and clamping/whitelisting to one of
[15,20,25,30] (or throw a descriptive Error if out of range); coerce gifLoop to
a strict boolean (Boolean(parsed.gifLoop)); keep existing enum checks for
format, quality, gifSizePreset but throw a clear error when values are invalid
so the caller fails fast rather than letting bad
width/height/gifFrameRate/gifLoop propagate to downstream functions that use
project, output, format, quality, width, height, gifFrameRate, gifSizePreset,
gifLoop.
| if (isCliRecord) { | ||
| try { | ||
| await runCliRecord(); | ||
| } catch (error) { | ||
| writeCliMessage("error", { | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| app.exit(1); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (isCliRender) { | ||
| try { | ||
| await runCliRender(); | ||
| } catch (error) { | ||
| writeCliMessage("error", { | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| app.exit(1); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm permission handlers are only registered in the non-CLI branch
rg -nP --type=ts -C2 'setPermission(Check|Request)Handler|askForMediaAccess' electron/Repository: siddharthvaddem/openscreen
Length of output: 1334
🏁 Script executed:
#!/bin/bash
# Check the exact content of the lines mentioned and surrounding context
sed -n '710,765p' electron/main.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 2095
🏁 Script executed:
#!/bin/bash
# Check if runCliRecord or runCliRender have any permission handling
rg -nP --type=ts 'function runCli(Record|Render)|const runCli(Record|Render)' -A 20 electron/Repository: siddharthvaddem/openscreen
Length of output: 2043
🏁 Script executed:
#!/bin/bash
# Find registerIpcHandlers definition and check for permission handling
rg -nP --type=ts 'function registerIpcHandlers|const registerIpcHandlers' -A 50 electron/ | head -100Repository: siddharthvaddem/openscreen
Length of output: 3142
🏁 Script executed:
#!/bin/bash
# Check if systemAudio or audio capture is used in CLI paths
rg -nP --type=ts 'systemAudio|getAudioSources|desktopCapturer' electron/ | grep -E '(runCli|handlers.ts)' -C 3Repository: siddharthvaddem/openscreen
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check how systemAudio is used and if it calls desktopCapturer
rg -nP --type=ts 'systemAudio' electron/ipc/handlers.ts -B 3 -A 10Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check what desktopCapturer.getSources is called with and around systemAudio
sed -n '660,680p' electron/ipc/handlers.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 832
🏁 Script executed:
#!/bin/bash
# Find the full handler that calls desktopCapturer.getSources
rg -nP --type=ts 'desktopCapturer.getSources' -B 30 electron/ipc/handlers.ts | head -60Repository: siddharthvaddem/openscreen
Length of output: 943
🏁 Script executed:
#!/bin/bash
# Look for how systemAudio config is used overall
rg -nP --type=ts 'systemAudio' electron/ -B 2 -A 2Repository: siddharthvaddem/openscreen
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Check how CLI record config (with systemAudio) is actually used to start recording
rg -nP --type=ts 'recordingConfig|cliRecordConfig' electron/main.ts -A 5 -B 2Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if desktopCapturer.getSources is called with systemAudio in the handlers
rg -nP --type=ts 'get-sources|get-cli-record-config' electron/ -A 3 -B 3Repository: siddharthvaddem/openscreen
Length of output: 1576
🏁 Script executed:
#!/bin/bash
# Check if there's renderer code in CLI mode that actually calls getSources
find electron -name "*.ts" -type f | xargs grep -l "getSources\|getCliRecordConfig" | head -5Repository: siddharthvaddem/openscreen
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Look for actual usage of systemAudio in any renderer or preload context
rg -nP --type=ts 'systemAudio.*getSources|getSources.*systemAudio|recordConfig.*systemAudio' electron/Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check the full runCliRecord implementation to see if it uses getSources
sed -n '518,620p' electron/main.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 3575
macOS first-run permission flow is kinda broken in CLI mode.
The early return statements skip the session.defaultSession.setPermissionCheckHandler / setPermissionRequestHandler registration and the systemPreferences.askForMediaAccess("microphone") prompt. When --cli-record --systemAudio is used on a fresh macOS install:
- Audio capture permission gets hit with no graceful pre-prompt — either silently fails or throws a janky single-use permission dialog mid-recording
- Screen recording is OS-mediated by
desktopCapturer.getSources()itself, so that's probably fine, but the missing handler still risks noisy warnings
At minimum, register the same permission handlers in the CLI paths. Ideally pre-prompt mic if systemAudio is requested. Running the CLI cold on macOS without ever launching the GUI is currently a confusing mess.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/main.ts` around lines 714 - 736, The CLI paths (isCliRecord /
isCliRender) return before registering
session.defaultSession.setPermissionCheckHandler and setPermissionRequestHandler
and before calling systemPreferences.askForMediaAccess("microphone"), causing
macOS first-run permission issues; move or duplicate the permission handler
registration and the microphone pre-prompt into the CLI branch before calling
runCliRecord/runCliRender (or extract to a helper used by both GUI and CLI), and
if the runCliRecord path detects the systemAudio flag, explicitly call
systemPreferences.askForMediaAccess("microphone") (and surface failures via
writeCliMessage("error", ...)) before awaiting runCliRecord so the OS pre-prompt
occurs when running headless.
| try { | ||
| try { | ||
| mediaStream = await navigator.mediaDevices.getUserMedia({ | ||
| audio: config.systemAudio | ||
| ? { | ||
| mandatory: { | ||
| chromeMediaSource: CHROME_MEDIA_SOURCE, | ||
| chromeMediaSourceId: source.id, | ||
| }, | ||
| } | ||
| : false, | ||
| video: videoConstraints, | ||
| } as unknown as MediaStreamConstraints); | ||
| } catch (error) { | ||
| if (!config.systemAudio) throw error; | ||
| sendToMain("status", { message: "System audio unavailable; retrying video-only..." }); | ||
| mediaStream = await navigator.mediaDevices.getUserMedia({ | ||
| audio: false, | ||
| video: videoConstraints, | ||
| } as unknown as MediaStreamConstraints); | ||
| } | ||
|
|
||
| const videoTrack = mediaStream.getVideoTracks()[0]; | ||
| if (!videoTrack) throw new Error("Video track is not available."); | ||
|
|
||
| try { | ||
| await videoTrack.applyConstraints({ | ||
| frameRate: { ideal: TARGET_FRAME_RATE, max: TARGET_FRAME_RATE }, | ||
| width: { ideal: TARGET_WIDTH, max: TARGET_WIDTH }, | ||
| height: { ideal: TARGET_HEIGHT, max: TARGET_HEIGHT }, | ||
| }); | ||
| } catch { | ||
| // Best-effort constraints; Electron may already choose the nearest valid mode. | ||
| } | ||
|
|
||
| const settings = videoTrack.getSettings(); | ||
| const width = Math.max(1, Math.floor((settings.width ?? 1920) / 2) * 2); | ||
| const height = Math.max(1, Math.floor((settings.height ?? 1080) / 2) * 2); | ||
| const mimeType = selectMimeType(); | ||
| recorderHandle = createRecorderHandle(mediaStream, { | ||
| mimeType, | ||
| videoBitsPerSecond: computeBitrate(width, height), | ||
| ...(mediaStream.getAudioTracks().length > 0 ? { audioBitsPerSecond: 192_000 } : {}), | ||
| }); | ||
|
|
||
| await window.electronAPI.setRecordingState(true, recordingId); | ||
| const startedAt = Date.now(); | ||
| await new Promise<void>((resolve) => window.setTimeout(resolve, config.durationMs)); | ||
| recorderHandle.recorder.stop(); | ||
| const recordedBlob = await recorderHandle.recordedBlobPromise; | ||
| const durationMs = Math.max(1, Date.now() - startedAt); | ||
| const fixedBlob = await fixWebmDuration(recordedBlob, durationMs); | ||
| const fileName = `recording-${recordingId}${VIDEO_FILE_EXTENSION}`; | ||
| const result = await window.electronAPI.storeRecordedSession({ | ||
| screen: { | ||
| videoData: await fixedBlob.arrayBuffer(), | ||
| fileName, | ||
| }, | ||
| createdAt: recordingId, | ||
| }); | ||
|
|
||
| if (!result.success || !result.path || !result.session) { | ||
| throw new Error(result.message || result.error || "Failed to store recording."); | ||
| } | ||
|
|
||
| sendToMain("done", { | ||
| path: result.path, | ||
| session: result.session, | ||
| durationMs, | ||
| source: { | ||
| id: source.id, | ||
| name: source.name, | ||
| display_id: source.display_id, | ||
| }, | ||
| }); | ||
| } finally { | ||
| try { | ||
| await window.electronAPI.setRecordingState(false); | ||
| } catch { | ||
| // The CLI result should not be masked by tray/state cleanup. | ||
| } | ||
| mediaStream?.getTracks().forEach((track) => track.stop()); | ||
| } |
There was a problem hiding this comment.
recorder cleanup is a bit fragile in the error path.
if setRecordingState(true) or getUserMedia rejects between starting the recorder and the await setTimeout, we'd throw and skip recorder.stop(). the finally only stops the underlying tracks, which on chrome doesn't reliably flush onstop with the final blob. lowkey risky for a CLI that's supposed to surface clean done/error states.
a small guard in finally plus a defensive try/catch around stop() (it throws InvalidStateError if the recorder already errored) would make this more robust:
♻️ proposed cleanup
} finally {
+ try {
+ if (recorderHandle && recorderHandle.recorder.state !== "inactive") {
+ recorderHandle.recorder.stop();
+ }
+ } catch {
+ // Recorder may have already errored; nothing actionable here.
+ }
try {
await window.electronAPI.setRecordingState(false);
} catch {
// The CLI result should not be masked by tray/state cleanup.
}
mediaStream?.getTracks().forEach((track) => track.stop());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/cli-record/CliRecordRenderer.tsx` around lines 121 - 203, The
finally block can skip stopping the MediaRecorder if an error occurs earlier, so
update the cleanup to first check recorderHandle and its recorder state (e.g.,
recorderHandle.recorder && recorderHandle.recorder.state !== "inactive") and
call recorderHandle.recorder.stop() inside a try/catch that swallows
InvalidStateError (and other errors) before stopping media tracks; then await
the recorderHandle.recordedBlobPromise (or handle its rejection) before calling
mediaStream.getTracks().forEach(track => track.stop()) and still call
window.electronAPI.setRecordingState(false) in a safe try/catch—this ensures
recorder.stop() runs defensively and the final blob gets flushed even on error
paths.
| if (config.format === "gif") { | ||
| const frameRate = (config.gifFrameRate ?? editor.gifFrameRate) as GifFrameRate; | ||
| const sizePreset = (config.gifSizePreset ?? editor.gifSizePreset) as GifSizePreset; | ||
| const loop = config.gifLoop ?? editor.gifLoop; | ||
| const dimensions = calculateOutputDimensions( | ||
| sourceWidth, | ||
| sourceHeight, | ||
| sizePreset, | ||
| GIF_SIZE_PRESETS, | ||
| aspectRatioValue, | ||
| ); | ||
| const exporter = new GifExporter({ | ||
| ...base, | ||
| width: dimensions.width, | ||
| height: dimensions.height, | ||
| frameRate, | ||
| loop, | ||
| sizePreset, | ||
| }); | ||
| await saveExport(await exporter.export(), config.output, "gif"); | ||
| return; |
There was a problem hiding this comment.
heads up: cli width/height are silently ignored for gif renders.
the parser in electron/main.ts accepts width/height for both mp4 and gif, but here gif dimensions only come from gifSizePreset. if a user passes --format gif --width 800 --height 600, those flags get silently dropped, which is the kind of thing that wastes someone's afternoon at 2am.
either honor the override (pass width/height into calculateOutputDimensions or build a custom dimensions object) or surface a warning via sendToMain("warning", ...) when they're set + format is gif.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/cli-render/CliRenderRenderer.tsx` around lines 238 - 258, The
GIF branch is ignoring CLI width/height flags, so update the gif handling in
CliRenderRenderer.tsx to honor user overrides: when config.format === "gif" read
config.width and config.height (falling back to calculateOutputDimensions like
currently done) and pass the resolved width/height into the new GifExporter
instead of only using gifSizePreset; reference calculateOutputDimensions and
GifExporter and use config.width/config.height as overrides for
dimensions.width/dimensions.height. Alternatively (or additionally) if you
prefer not to override presets, detect when config.width or config.height is set
with format "gif" and call sendToMain("warning", ...) to surface a warning to
the user that CLI width/height are ignored for GIFs.
1 participant
Pull Request Template
Description
Motivation
Type of Change
Related Issue(s)
Screenshots / Video
Screenshot (if applicable):
Video (if applicable):
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit
Release Notes
openscreen) for recording and rendering videos from the command line